8369609: Continuations preempt_epilog is missing a call to invalidate_jvmti_stack #27878

sspitsyn · 2025-10-17T20:05:24Z

This is a simple fix to add missing call to invalidate_jvmti_stack() to the freeze_epilog(ContinuationWrapper& cont).

Testing:

TBD: Run mach5 tiers 1-6

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8369609: Continuations preempt_epilog is missing a call to invalidate_jvmti_stack (Bug - P4)

Reviewers

Patricio Chilano Mateo (@pchilano - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/27878/head:pull/27878
$ git checkout pull/27878

Update a local copy of the PR:
$ git checkout pull/27878
$ git pull https://git.openjdk.org/jdk.git pull/27878/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 27878

View PR using the GUI difftool:
$ git pr show -t 27878

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/27878.diff

Using Webrev

Link to Webrev Comment

…_jvmti_stack

bridgekeeper · 2025-10-17T20:06:57Z

👋 Welcome back sspitsyn! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2025-10-17T20:07:10Z

@sspitsyn This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8369609: Continuations preempt_epilog is missing a call to invalidate_jvmti_stack

Reviewed-by: pchilanomate

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 30 new commits pushed to the master branch:

cc9483b: 8366182: Some PKCS11Tests are being skipped when they shouldn't
470eedb: 8370511: test/jdk/javax/swing/JSlider/bug4382876.java does not release previously pressed keys
f73e56e: 8361894: sun/security/krb5/config/native/TestDynamicStore.java ensure that the test is run with sudo
... and 27 more: https://git.openjdk.org/jdk/compare/d8ebe387595af43e2cdbbce396547d6daaf8c7dc...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

openjdk · 2025-10-17T20:08:19Z

@sspitsyn The following label will be automatically applied to this pull request:

hotspot-runtime

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2025-10-17T20:11:14Z

Webrevs

pchilano

Thanks for fixing this Serguei.

pchilano · 2025-10-20T21:59:40Z

src/hotspot/share/runtime/continuationFreezeThaw.cpp


  log_develop_debug(continuations)("=== End of freeze cont ### #" INTPTR_FORMAT, cont.hash());
+
+  JVMTI_ONLY(invalidate_jvmti_stack(JavaThread::current()));


Why not directly in preempt_epilog? Is it to cover the freeze fast case too? If that’s the case we should remove the call to invalidate_jvmti_stack from jvmti_yield_cleanup to avoid calling it twice for the freeze slow case. Also I wonder if this call to invalidate_jvmti_stack should just be moved to JvmtiVTMSTransitionDisabler::VTMS_unmount_end instead.

Thank you for the comment! Yes, I had in mind but forgot to remove the call to invalidate_jvmti_stack() from jvmti_yield_cleanup(). I've pushed the update now.

Also I wonder if this call to invalidate_jvmti_stack should just be moved to JvmtiVTMSTransitionDisabler::VTMS_unmount_end instead.

Unfortunately, this is not going to work for plain/pure continuations as mount/unmount code path does not work for them.

I see. What do you think about adding invalidate_jvmti_stack in jvmti_yield_cleanup if !cont.entry()->is_virtual_thread() is true to address that case?

Good idea! It'd be nice to move the invalidate_jvmti_stack() calls out of the continuation code. I'll test it and let you know the results.

Strangely, the test serviceability/jvmti/vthread/ContStackDepthTest is failing with the assert(_cur_stack_depth == num_frames). Obviously, some of the code paths is missed to call invalidate_jvmti_stack().

Ah, we need to call invalidate_jvmti_stack at the end in jvmti_yield_cleanup! The problem is that in JvmtiExport::continuation_yield_cleanup we are setting again the stack depth when calling state->cur_stack_depth(). We count the enterSpecial frame at the top but we don’t decrement the depth when removing it (done in assembly code).

Okay, thanks! Did you verify it with run of test serviceability/jvmti/vthread/ContStackDepthTest? I see it is still failing with the call to invalidate_jvmti_stack() at the end of jvmti_yield_cleanup().

This test does not fail with the following patch:

diff --git a/src/hotspot/share/runtime/continuationFreezeThaw.cpp b/src/hotspot/share/runtime/continuationFreezeThaw.cpp index 0750f611876..a12e99903af 100644 --- a/src/hotspot/share/runtime/continuationFreezeThaw.cpp +++ b/src/hotspot/share/runtime/continuationFreezeThaw.cpp @@ -1626,12 +1626,17 @@ static void invalidate_jvmti_stack(JavaThread* thread) { } static void jvmti_yield_cleanup(JavaThread* thread, ContinuationWrapper& cont) { - if (!cont.entry()->is_virtual_thread() && JvmtiExport::has_frame_pops(thread)) { + if (cont.entry()->is_virtual_thread()) { + return; + } + invalidate_jvmti_stack(thread); + if (JvmtiExport::has_frame_pops(thread)) { int num_frames = num_java_frames(cont); ContinuationWrapper::SafepointOp so(Thread::current(), cont); - JvmtiExport::continuation_yield_cleanup(JavaThread::current(), num_frames); + JvmtiExport::continuation_yield_cleanup(thread, num_frames); } + invalidate_jvmti_stack(thread); } static void jvmti_mount_end(JavaThread* current, ContinuationWrapper& cont, frame top) { @@ -1685,7 +1690,6 @@ static inline freeze_result freeze_epilog(ContinuationWrapper& cont) { log_develop_debug(continuations)("=== End of freeze cont ### #" INTPTR_FORMAT, cont.hash()); - JVMTI_ONLY(invalidate_jvmti_stack(JavaThread::current())); return freeze_ok; } @@ -2311,7 +2315,7 @@ NOINLINE intptr_t* Thaw<ConfigT>::thaw_slow(stackChunkOop chunk, Continuation::t assert(_cont.chunk_invariant(), ""); - JVMTI_ONLY(invalidate_jvmti_stack(_thread)); + JVMTI_ONLY(if (!_cont.entry()->is_virtual_thread()) invalidate_jvmti_stack(_thread)); _thread->set_cont_fastpath(_fastpath);

The call to invalidate_jvmti_stack() is still needed in the thaw_slow().
I can go ahead with this update if you are okay with it.

This is the patch I tested out with serviceability/jvmti/vthread/ContStackDepthTest (on top of mainline):

diff --git a/src/hotspot/share/prims/jvmtiThreadState.cpp b/src/hotspot/share/prims/jvmtiThreadState.cpp index 00a48dec111..181d77f4d10 100644 --- a/src/hotspot/share/prims/jvmtiThreadState.cpp +++ b/src/hotspot/share/prims/jvmtiThreadState.cpp @@ -678,6 +678,10 @@ void JvmtiVTMSTransitionDisabler::VTMS_unmount_end(jobject vthread) { JavaThread* thread = JavaThread::current(); assert(thread->is_in_VTMS_transition(), "sanity check"); + JvmtiThreadState *state = thread->jvmti_thread_state(); + if (state != nullptr) { + state->invalidate_cur_stack_depth(); + } finish_VTMS_transition(vthread, /* is_mount */ false); } diff --git a/src/hotspot/share/runtime/continuationFreezeThaw.cpp b/src/hotspot/share/runtime/continuationFreezeThaw.cpp index 3e509e71551..a493fca076e 100644 --- a/src/hotspot/share/runtime/continuationFreezeThaw.cpp +++ b/src/hotspot/share/runtime/continuationFreezeThaw.cpp @@ -1626,13 +1626,15 @@ static void invalidate_jvmti_stack(JavaThread* thread) { } static void jvmti_yield_cleanup(JavaThread* thread, ContinuationWrapper& cont) { - if (!cont.entry()->is_virtual_thread() && JvmtiExport::has_frame_pops(thread)) { - int num_frames = num_java_frames(cont); + if (!cont.entry()->is_virtual_thread()) { + if (JvmtiExport::has_frame_pops(thread)) { + int num_frames = num_java_frames(cont); - ContinuationWrapper::SafepointOp so(Thread::current(), cont); - JvmtiExport::continuation_yield_cleanup(JavaThread::current(), num_frames); + ContinuationWrapper::SafepointOp so(Thread::current(), cont); + JvmtiExport::continuation_yield_cleanup(JavaThread::current(), num_frames); + } + invalidate_jvmti_stack(thread); } - invalidate_jvmti_stack(thread); } static void jvmti_mount_end(JavaThread* current, ContinuationWrapper& cont, frame top) {

Originally I had invalidate_jvmti_stack() before calling JvmtiExport::continuation_yield_cleanup so I could reproduce the issue.

I think you are right that for virtual threads we shouldn't need to invalidate the stack, so your patch is an improvement.

Okay, thanks! I've pushed my latest changes which are aligned with your patch above and invalidate the stack for plain/pure continuations only. So, the VTMS_unmount_end() still does not include it.

pchilano

Looks good to me, thanks Serguei!

sspitsyn · 2025-10-24T17:36:49Z

Patricio, thank you a lot for review a suggestions!

8369609: Continuations preempt_epilog is missing a call to invalidate…

c30793f

…_jvmti_stack

openjdk bot added the hotspot-runtime [email protected] label Oct 17, 2025

openjdk bot added the rfr Pull request is ready for review label Oct 17, 2025

pchilano reviewed Oct 20, 2025

View reviewed changes

sspitsyn added 4 commits October 21, 2025 18:49

review: remove now unneeded call to invalidate_jvmti_stack

1b5a057

Merge

3edebdb

review: call to invalidate_jvmti_stack for pure continuations only

e6fc405

remove unintentionally added new empty line

4a4302b

pchilano approved these changes Oct 24, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Oct 24, 2025


		log_develop_debug(continuations)("=== End of freeze cont ### #" INTPTR_FORMAT, cont.hash());

		JVMTI_ONLY(invalidate_jvmti_stack(JavaThread::current()));

8369609: Continuations preempt_epilog is missing a call to invalidate_jvmti_stack #27878

Are you sure you want to change the base?

8369609: Continuations preempt_epilog is missing a call to invalidate_jvmti_stack #27878

Conversation

sspitsyn commented Oct 17, 2025 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Oct 17, 2025

Uh oh!

openjdk bot commented Oct 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Oct 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mlbridge bot commented Oct 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

pchilano left a comment

Choose a reason for hiding this comment

Uh oh!

pchilano Oct 20, 2025

Choose a reason for hiding this comment

Uh oh!

sspitsyn Oct 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pchilano Oct 22, 2025

Choose a reason for hiding this comment

Uh oh!

sspitsyn Oct 22, 2025

Choose a reason for hiding this comment

Uh oh!

sspitsyn Oct 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pchilano Oct 23, 2025

Choose a reason for hiding this comment

Uh oh!

sspitsyn Oct 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sspitsyn Oct 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pchilano Oct 23, 2025

Choose a reason for hiding this comment

Uh oh!

sspitsyn Oct 24, 2025

Choose a reason for hiding this comment

Uh oh!

pchilano left a comment

Choose a reason for hiding this comment

Uh oh!

sspitsyn commented Oct 24, 2025

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants

sspitsyn commented Oct 17, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Oct 17, 2025 •

edited

Loading

openjdk bot commented Oct 17, 2025 •

edited

Loading

mlbridge bot commented Oct 17, 2025 •

edited

Loading

sspitsyn Oct 22, 2025 •

edited

Loading

sspitsyn Oct 22, 2025 •

edited

Loading

sspitsyn Oct 23, 2025 •

edited

Loading

sspitsyn Oct 23, 2025 •

edited

Loading